-
Notifications
You must be signed in to change notification settings - Fork 175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding D11 support for nextjs. #778
Conversation
@apathak18 is attempting to deploy a commit to the Chapter Three Team on Vercel. A member of the Team first needs to authorize it. |
@shadcn I'm not able to add the reviewer and other pipelines are blocked without reviewer. |
@shadcn could you please review this PR and check if its looks for D11 compatible release? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a quick review, this is the only thing I see as an issue.
@JohnAlbin, |
@JohnAlbin Please review this PR and let me know if any other changes required. |
- drupal: "11.0.x" | ||
php: "8.1" | ||
- drupal: "11.0.x" | ||
php: "8.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
11.0 requires PHP 8.3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yobottehg Correct!!
Pipeline is running on 8.3
see:
this configuration is to exclude running with 8.1, 8.2 versions of php for Drupal 11
'@action' => $action, | ||
]; | ||
|
||
$this->assertNotEmpty(Database::getConnection()->select('watchdog', 'w') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@apathak18 I'm not sure on this, but we can use it like below instead of using Database
class:
$this->assertNotEmpty(Database::getConnection()->select('watchdog', 'w') | |
$this->assertNotEmpty($this->container->get('database')->select('watchdog', 'w') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally things look good here. Thanks to all who contributed on this one.
In general, I'd encourage the scope of an upgrade issue like this to be kept just to things necessary for the upgrade - core version requirement in module info files, any deprecated code. There were some things along the way outside of that, and I still think some of the changes to the NextSettingsForm are not related to D11 compatibility. Don't doubt that these could be worthwhile changes, but keeping them in a separate PR will hopefully make it easier for us to get important things Drupal version compatibility through more quickly.
I confirmed that the module tests are passing locally, and also did some manual e2e testing.
There are still sone deprecation warnings thrown by the module tests, but I'll create a follow up PR for that so we can merge this.
This feedback has been resolved.
This has been released as 2.0.0-beta1: https://www.drupal.org/project/next/releases/2.0.0-beta1 |
This pull request is for: (mark with an "x")
examples/*
modules/next
packages/next-drupal
starters/basic-starter
starters/graphql-starter
starters/pages-starter
GitHub Issue: #
Please add a link to the GitHub issue: #777
where this problem is discussed.
Getting ready for this
Code changes need test coverage. If you don't know
how to make tests, check this box to ask for help.
Describe your changes
A clear and concise description of what the request is.
If applicable, add screenshots to help explain your issue.